Add REST API query normalization #9190
Conversation
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
edwin-greene
added
enhancement
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9190 +/- ##
============================================
+ Coverage 92.33% 92.55% +0.22%
+ Complexity 7600 6997 -603
============================================
Files 915 903 -12
Lines 30503 29643 -860
Branches 3727 3745 +18
============================================
- Hits 28165 27437 -728
+ Misses 1506 1442 -64
+ Partials 832 764 -68 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
|
There was a problem hiding this comment.
I really like this. I can import normalizeRequestQueryParams() into the new response caching middleware and use it to form most of the cache key, along with Accept-Encoding header value.
As far as this PR goes, it can be merged independently as no code is invoked as configured Express middleware or as a query processor.
| input: { | ||
| path: '/api/v1/blocks', | ||
| query: { | ||
| order: 'asc', | ||
| unknown: '3', | ||
| limit: '5', | ||
| }, | ||
| }, | ||
| expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', |
There was a problem hiding this comment.
For clarity and brevity, it would be better if input was simply a string even if you have to parse out the query params from that string in the test. Also, should provide a description that's relayed to jest.
| input: { | |
| path: '/api/v1/blocks', | |
| query: { | |
| order: 'asc', | |
| unknown: '3', | |
| limit: '5', | |
| }, | |
| }, | |
| expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', | |
| description: 'Query parameters are sorted', | |
| input: '/api/v1/blocks?order=asc&unknown=3&limit=5', | |
| expected: '/api/v1/blocks?limit=5&order=asc&unknown=3', |
| if (_.isUndefined(openApiMap)) { | ||
| const openApiObject = getV1OpenApiObject(); | ||
| const patternMap = getPathParametersPatterns(openApiObject); | ||
| openApiMap = new Map(); |
There was a problem hiding this comment.
Should not assign openApiMap until after it's populated. Use a temporary variable.
| * @param patternMap | ||
| * @returns {RegExp} | ||
| */ | ||
| const pathToRegexConverter = (path, patternMap) => { |
There was a problem hiding this comment.
Instead of searching 20+ regexes to map it to OpenAPI, we should enable the OpenAPI middleware which already attaches an OpenAPI object to each request. This should be faster than our approach. We should take care to disable request and response validation outside tests though.
Description:
Adds a means to normalize REST API queries.
Related issue(s):
Fixes #9113
Notes for reviewer:
Checklist